Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Evaluation Documentation #452

Conversation

christophertubbs
Copy link
Contributor

Documentation for all aspects of evaluations

… scoring.py and filled out the readme for metrics.
@robertbartel robertbartel added documentation Improvements or additions to documentation maas MaaS Workstream labels Oct 16, 2023
@christophertubbs christophertubbs marked this pull request as ready for review October 24, 2023 14:37
@christophertubbs
Copy link
Contributor Author

There's more to be done, but I'm opening this up for review just so we can get stuff in there - perfection is the enemy of progress and all that.

@aaraney
Copy link
Member

aaraney commented Oct 24, 2023

Looking at this now.

Copy link
Member

@aaraney aaraney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, @christophertubbs! I left a few comments, several of which are minor grammatical errors and others are optional suggestions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming this is what you are working on now.

python/lib/metrics/README.md Show resolved Hide resolved
`Communicator` objects to help distribute information generated during evaluation. Next, you can
call the `ScoringScheme` as a function with prepared pairs, where to find "observed" values within
the pairs, where to find the "predicted" values within the pairs, and thresholds. This invocation
will yield a `MetricResults` object.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a minimum viable example you could provider here? If its more than, say 5 lines, maybe it is not worth it. But it would nice to see all the concepts you described above in action. I think it is okay to let the reader imagine the data flowing through the system.


Each `MetricResults` object contains the evaluated metrics performed on a singular set of data.
When providing the `Metric`s that are run to the `ScoringScheme`, a weight is passed along for
each metric and passing thresholds to the invocation will provide weights for each threshold. This
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
each metric and passing thresholds to the invocation will provide weights for each threshold. This
each metric. Passing thresholds to the invocation will provide weights for each threshold. This

Each `MetricResults` object contains the evaluated metrics performed on a singular set of data.
When providing the `Metric`s that are run to the `ScoringScheme`, a weight is passed along for
each metric and passing thresholds to the invocation will provide weights for each threshold. This
provides a basis for determine a hierarchy of importance for each metric and threshold. For an
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
provides a basis for determine a hierarchy of importance for each metric and threshold. For an
provides a basis to establish a hierarchy of importance for each metric and threshold. For an

This means that the `info` event will be triggered on each held `Communicator`, but only on those set to handle
messages of a verbosity of `LOUD` or greater, and to call the `write` event after doing so.

Say I have three communicators:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be super nice to see how these three example Communicators are created / configured.


Templates are environment specific - one environment may have an important template while another might not,
but the templates are configurable, so more and more may be created as new use cases arise. Template Manager
constructs (such as the [FileTemplateManager](template.py)) provide all the means necessary to find out what is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
constructs (such as the [FileTemplateManager](template.py)) provide all the means necessary to find out what is
constructs (such as the [FileTemplateManager](template.py)) provide all the means necessary to find out what templates are

The most important aspects defined by a `DataSourceSpecification` are:

1. What fields to load
2. What the data is measured in or how to find out
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are udunits supported? Or what is/are the expected unit type formats?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am now getting to the unit definitions section below. It might be worth linking that that section here. Thoughts?

}
```

Retrieve data "path/to/file.json" in the style handled by the "JSON File" template
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am assuming the JSON File template is provided and always available? Likewise, the same can be said for the Instantaneous NWIS Streamflow template, correct?

```

<a id="UnitDefinition"></a>
## Unit Definition
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a list of supported units / a dependency (i.e. pint) that you can point to in this section?

Copy link
Member

@aaraney aaraney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working through the changes with me! Merge at will

@christophertubbs christophertubbs merged commit 7c9f034 into NOAA-OWP:master Oct 24, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation maas MaaS Workstream
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants